Conversation
WalkthroughAdds exported test lifecycle hooks and object-initialization stubs across two test suites to start/stop an HttpBin service, set an environment flag, and preserve/restore per-test environment state. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/HttpBin_API_test.os (1)
17-41: Harden lifecycle hooks: guardОстановить()and strengthen “service is running” checks.
Right nowПослеЗапускаТестов()unconditionally callsHttpBin.Остановить()(can throw ifПередЗапускомТестов()failed early /HttpBinisНеопределено). AlsoПередЗапускомТеста()only checksСервисЗапущен, which can drift from reality.&ПослеВсех Процедура ПослеЗапускаТестов() Экспорт - HttpBin.Остановить(); + Если HttpBin <> Неопределено Тогда + HttpBin.Остановить(); + КонецЕсли; + СервисЗапущен = Ложь; КонецПроцедуры &ПередКаждым Процедура ПередЗапускомТеста() Экспорт - Если Не СервисЗапущен Тогда + Если Не СервисЗапущен Или HttpBin = Неопределено Тогда ВызватьИсключение "Сервис не запущен"; КонецЕсли; КонецПроцедурыAlso consider restoring
HTTPBIN_IS_TEST_MODEinПослеЗапускаТестов()to avoid leaking state into other suites.tests/HttpBin_test.os (2)
11-30: Avoid double-stop and keep_HttpBinstate consistent inПослеЗапускаТеста().
ПослеЗапускаТеста()stops_HttpBinbut doesn’t clear it; plus one test stops it manually, so teardown may stop twice.&ПослеКаждого Процедура ПослеЗапускаТеста() Экспорт УстановитьТекущийКаталог(_ТекущийКаталог); Если Не _HttpBin = Неопределено Тогда _HttpBin.Остановить(); + _HttpBin = Неопределено; КонецЕсли; КонецПроцедуры
43-64: Async-start test is flaky: replace time-based sleeps with “wait until отвечает() (with timeout)”.
UsingВремяЗапускаfrom a prior run +Приостановить(ВремяЗапуска)will intermittently fail on CI variance; also you currently stop_HttpBinin-test and then teardown stops again unless you clear_HttpBin.Процедура ТестДолжен_ЗапуститьСервисАсинхронноИОстановить() Экспорт @@ _HttpBin.Запустить(); @@ - _HttpBin.Остановить(); + _HttpBin.Остановить(); + _HttpBin = Неопределено; // чтобы teardown не пытался остановить повторно @@ // Действие _HttpBin.ЗапуститьАсинх(); - - Приостановить(ВремяЗапуска); + // TODO: дождаться запуска (poll) с таймаутом, вместо sleep по прошлому времени // Проверка Ожидаем.Что(_HttpBin.Отвечает(), "Должен быть запущен").ЭтоИстина();(Я бы вынес общий helper
ДождатьсяЗапускаHttpBin(_HttpBin, ТаймаутМс)и переиспользовал его.)
🧹 Nitpick comments (1)
tests/HttpBin_API_test.os (1)
12-15: EmptyПриСозданииОбъекта()looks redundant—either remove or add a brief comment why it’s required by oneunit.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/HttpBin_API_test.os (1)
31-34: Consider adding a guard before stopping the service.If
ПередЗапускомТестов()fails beforeHttpBinis assigned (e.g., exception during initialization), callingHttpBin.Остановить()could fail. The pattern inHttpBin_test.os(line 26-28) checksЕсли Не _HttpBin = Неопределеноbefore stopping.&ПослеВсех Процедура ПослеЗапускаТестов() Экспорт + Если Не HttpBin = Неопределено Тогда HttpBin.Остановить(); + КонецЕсли; КонецПроцедуры
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/HttpBin_API_test.os(2 hunks)tests/HttpBin_test.os(1 hunks)
🔇 Additional comments (6)
tests/HttpBin_test.os (3)
11-14: LGTM: Test set annotation and object initialization stub.The
&ТестовыйНабор(Характер = "Одиночка")annotation correctly marks this as a singleton test set for oneunit. The emptyПриСозданииОбъекта()procedure is an appropriate stub required by the framework.
16-19: LGTM: Per-test setup hook.The
&ПередКаждымannotation correctly marks the existing setup procedure for per-test execution. Saving the current directory before each test enables proper cleanup.
21-30: LGTM: Per-test teardown hook.The
&ПослеКаждогоannotation and teardown logic are correct. The procedure properly restores the working directory and conditionally stops the HttpBin service if it was initialized.tests/HttpBin_API_test.os (3)
12-15: LGTM: Test set annotation and object initialization stub.Consistent with the pattern in
HttpBin_test.os.
17-29: LGTM: Global test setup.The
&ПередВсемиhook correctly initializes the HttpBin service once for all tests and sets the test mode environment variable. TheСервисЗапущенflag provides state tracking for the per-test guard.
36-41: LGTM: Per-test service availability guard.The
&ПередКаждымhook properly checks if the service started successfully and throws a clear exception if not, preventing confusing failures in individual tests.
|
Спасибо! |
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.